Skip to content

Nest x-medkit vendor fields on SOVD endpoint payloads#390

Merged
bburda merged 11 commits intomainfrom
feat/issue-385-nest-x-medkit-vendor-fields
Apr 29, 2026
Merged

Nest x-medkit vendor fields on SOVD endpoint payloads#390
bburda merged 11 commits intomainfrom
feat/issue-385-nest-x-medkit-vendor-fields

Conversation

@bburda
Copy link
Copy Markdown
Collaborator

@bburda bburda commented Apr 26, 2026

Summary

Migrates the two remaining SOVD-standard endpoint payloads that emit flat
x-medkit-* top-level vendor keys to the nested x-medkit: {...}
convention that every other endpoint already follows.

  • GET /updates/{id}/status -> x-medkit.phase (was flat x-medkit-phase)
  • GET /apps|components/{id}/configurations -> nested x-medkit.source
    on items, x-medkit.{source,node} on per-parameter entries (was flat
    x-medkit-source / x-medkit-node)

The OpenAPI schemas for UpdateStatus and ConfigurationMetaData now
declare the nested object so generated clients pick up accurate typing.

Adds test_openapi_response_drift - a runtime drift validator that
walks every GET endpoint declared in GET /docs, fetches a real
response, and validates it against the declared response schema. The
original x-medkit-phase divergence slipped past every typed client
because the schema never declared the field; this test catches that
class of regression. The compile-time emitter -> schema contract is
deferred to #338.


Issue


Type

  • Bug fix
  • New feature or tests
  • Breaking change
  • Documentation only

Breaking on the wire format of two endpoints. Cross-repo grep showed
zero current consumers reading the legacy flat keys (web UI, MCP,
foxglove, demos, generated clients, commercial).


Testing

  • colcon build --packages-up-to ros2_medkit_gateway ros2_medkit_integration_tests (Jazzy, Release)
  • colcon test --packages-select ros2_medkit_gateway --ctest-args -L linter -> green
  • ctest -R test_update_manager -> 24/24 passing (UpdateStatusToJson + UpdateManagerFailureTest assertions updated)
  • Smoke: GET /api/v1/docs returns UpdateStatus.properties.x-medkit schema with phase enum
  • Drift integration test (test_openapi_response_drift) runs in CI - requires python3-jsonschema (added to ros2_medkit_integration_tests/package.xml)

Checklist

  • Breaking changes are clearly described
  • Tests were added or updated if needed
  • Docs were updated (docs/api/rest.rst example payload + vendor extension prose)

Per #385, vendor extensions on SOVD-standard endpoint payloads should be
nested under a single `x-medkit` object instead of flat top-level keys.
This was already the convention for most endpoints (faults, data, ops,
bulk-data, ...) but two sites still emitted flat keys:

- GET /updates/{id}/status emitted `x-medkit-phase` at the top level
- GET /apps|components/{id}/configurations emitted `x-medkit-source`
  on each item and `x-medkit-source`/`x-medkit-node` on each parameter

Both now emit a nested `x-medkit` object. The OpenAPI schemas for
UpdateStatus and ConfigurationMetaData declare the nested object
explicitly so generated clients (ros2_medkit_clients) get accurate
typing.

Adds an integration test (test_openapi_response_drift) that validates
runtime GET responses against the OpenAPI response schema served at
/api/v1/docs. The original drift slipped past every typed client because
the schema never declared `x-medkit-phase` even though the handler
emitted it; this test catches that class of regression at runtime.
The compile-time contract (template-based emitter-to-schema link) lands
later under #338.
Copilot AI review requested due to automatic review settings April 26, 2026 15:56
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR standardizes remaining SOVD-standard payloads to use nested x-medkit: { ... } vendor extensions (instead of flat x-medkit-* keys) and strengthens spec/implementation consistency by adding an integration drift test that validates live GET responses against the runtime OpenAPI 3.1 schema served at /docs.

Changes:

  • Migrate update status payload vendor extension from x-medkit-phase to x-medkit.phase and update schema/docs/tests accordingly.
  • Migrate configurations payload vendor extension fields to nested x-medkit objects (per-item and per-parameter metadata).
  • Add test_openapi_response_drift integration test and the python3-jsonschema test dependency.

Reviewed changes

Copilot reviewed 8 out of 8 changed files in this pull request and generated 2 comments.

Show a summary per file
File Description
src/ros2_medkit_integration_tests/test/features/test_openapi_response_drift.test.py New integration test validating live GET JSON responses against /docs OpenAPI response schemas.
src/ros2_medkit_integration_tests/package.xml Adds python3-jsonschema as a test dependency for the drift validator.
src/ros2_medkit_gateway/test/test_update_manager.cpp Updates unit tests to assert nested x-medkit.phase.
src/ros2_medkit_gateway/src/updates/update_manager.cpp Comment updates to reflect nested vendor extension naming.
src/ros2_medkit_gateway/src/openapi/schema_builder.cpp Updates OpenAPI schemas to declare nested x-medkit for UpdateStatus + ConfigurationMetaData.
src/ros2_medkit_gateway/src/http/handlers/config_handlers.cpp Emits nested x-medkit metadata instead of flat x-medkit-source/x-medkit-node.
src/ros2_medkit_gateway/include/ros2_medkit_gateway/updates/update_types.hpp Switches UpdateStatus JSON serialization to nested x-medkit.phase.
docs/api/rest.rst Updates REST docs examples/prose for nested x-medkit.phase.

@bburda bburda marked this pull request as draft April 26, 2026 18:12
@bburda bburda self-assigned this Apr 26, 2026
bburda added 5 commits April 27, 2026 09:48
GET /apps/{id}/is-located-on returns a single-element collection
({items, x-medkit, _links}) per the handler in discovery_handlers.cpp,
but the spec was advertising it as EntityDetail (single object with
required id/name). Aligns the spec to the actual response shape so the
new drift test passes and generated clients get the right type.
ProcInfoHandler iteration yields ProcessExited events, not name keys -
indexing back into proc_info[proc] raised KeyError on every shutdown.
Mirrors the pattern used in test_openapi_callability.
Ubuntu 22.04 ships python3-jsonschema 3.2 which lacks Draft202012Validator
(added in 4.0). The validation surface we use (required, type, properties)
behaves identically across drafts, so falling back to Draft7 keeps the
drift test working on Humble while preferring Draft202012 on Jazzy/Rolling
for OpenAPI 3.1 alignment.
Cold-cache ASan builds take ~33 min on the GitHub-hosted runner, which
left ~12 min for the integration test pass. PR branches that miss the
ccache restore-keys lookup were getting cancelled at the last scenario
test before the actions/cache post step could persist the build, so
each subsequent run hit the same cold cache. 60 min provides clear
headroom to complete + save cache once, after which warm-cache runs
will return to the typical ~22 min.
SOVD does not require vendor extensions, and the existing codebase
convention (fault_detail_schema, entity_detail_schema) leaves x-medkit
out of the required list even when the gateway always emits it. Marking
it required in update_status_schema would imply a SOVD-incompatible
contract while only the gateway implementation actually depends on it.
The explicit handler guard (test_update_status_payload_uses_nested_x_medkit)
keeps regression risk covered.
@bburda bburda marked this pull request as ready for review April 27, 2026 13:58
@bburda bburda requested review from Copilot and mfaferek93 April 27, 2026 13:58
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR standardizes remaining flat x-medkit-* vendor keys on SOVD-standard payloads into the existing nested "x-medkit": {...} convention, updates the runtime OpenAPI schemas accordingly, and adds an integration test intended to detect handler-vs-schema response drift.

Changes:

  • Migrate update status vendor field from flat x-medkit-phase to nested x-medkit.phase (code, schema, docs, and tests).
  • Migrate configurations vendor fields to nested x-medkit objects (items and per-parameter entries) and update ConfigurationMetaData schema.
  • Add test_openapi_response_drift and add python3-jsonschema as an integration-test dependency.

Reviewed changes

Copilot reviewed 10 out of 10 changed files in this pull request and generated 6 comments.

Show a summary per file
File Description
src/ros2_medkit_integration_tests/test/features/test_openapi_response_drift.test.py New integration test that fetches /docs, calls GET endpoints, and validates 200 JSON bodies against response schemas.
src/ros2_medkit_integration_tests/package.xml Adds python3-jsonschema test dependency for the drift validator.
src/ros2_medkit_gateway/test/test_update_manager.cpp Updates assertions for nested x-medkit.phase in update-status JSON.
src/ros2_medkit_gateway/src/updates/update_manager.cpp Updates comments to reflect nested x-medkit.phase convention.
src/ros2_medkit_gateway/src/openapi/schema_builder.cpp Updates UpdateStatus and ConfigurationMetaData schemas to declare nested x-medkit objects.
src/ros2_medkit_gateway/src/http/rest_server.cpp Fixes spec response typing for GET /apps/{id}/is-located-on to match actual list payload shape.
src/ros2_medkit_gateway/src/http/handlers/config_handlers.cpp Emits nested x-medkit objects for aggregated configuration metadata and per-parameter provenance.
src/ros2_medkit_gateway/include/ros2_medkit_gateway/updates/update_types.hpp Changes update status JSON serialization to emit nested x-medkit.phase.
docs/api/rest.rst Updates REST docs examples and prose to use nested x-medkit.phase.
.github/workflows/quality.yml Increases CI job timeout to accommodate cold-cache ASan builds.

Comment thread src/ros2_medkit_gateway/test/test_update_manager.cpp Outdated
Comment thread src/ros2_medkit_gateway/test/test_update_manager.cpp
Comment thread src/ros2_medkit_gateway/test/test_update_manager.cpp Outdated
Comment thread src/ros2_medkit_gateway/test/test_update_manager.cpp
Four review points addressed:

- test_update_manager.cpp: switch j["x-medkit"]["phase"] reads to
  contains() + at(). nlohmann's operator[] on a non-const json inserts
  missing keys, which can mask a regression where update_status_to_json
  stops emitting the vendor extension.
- test_openapi_response_drift: REQUIRED_APPS = {temp_sensor, calibration}
  forces deterministic discovery before setUpClass proceeds, instead of
  relying on MIN_EXPECTED_APPS=1 catching an arbitrary first node.
- _inline_refs: raise _RefResolutionError on unresolved or cyclic $refs
  rather than returning {} (which validates anything and hides drift).
  Caught in the validation loop so multiple spec issues surface in one
  run.
- Module docstring: spell out what the validator catches (required,
  type, enum, nested shape) and what it does not (extra undeclared
  fields, since most schemas don't set additionalProperties: false).
  Closing the additionalProperties gap belongs in the issue #338 rework.
@bburda bburda requested a review from Copilot April 27, 2026 16:00
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Pull request overview

This PR standardizes remaining SOVD endpoint vendor extensions to the nested "x-medkit": {...} convention (instead of flat x-medkit-* keys), updates the OpenAPI schemas accordingly, and adds an integration test to detect schema/handler response drift.

Changes:

  • Migrate update status vendor field from x-medkit-phase to nested x-medkit.phase (payload + schema + tests + docs).
  • Migrate configurations vendor fields to nested x-medkit (source on list items; source/node on per-parameter entries in the vendor extension payload).
  • Add an integration drift test that validates live GET responses against the runtime /docs JSON Schemas; extend CI timeout to accommodate slower cold-cache ASan runs.

Reviewed changes

Copilot reviewed 10 out of 10 changed files in this pull request and generated 1 comment.

Show a summary per file
File Description
src/ros2_medkit_integration_tests/test/features/test_openapi_response_drift.test.py New integration test that walks GET endpoints from /docs and validates 200 JSON responses against declared schemas; includes a targeted guard for nested x-medkit.phase on update status.
src/ros2_medkit_integration_tests/package.xml Adds python3-jsonschema as a test dependency to support the drift validator.
src/ros2_medkit_gateway/test/test_update_manager.cpp Updates unit tests to assert nested x-medkit.phase using contains()/at() (non-mutating access).
src/ros2_medkit_gateway/src/updates/update_manager.cpp Updates comments to reflect the new nested vendor extension naming.
src/ros2_medkit_gateway/src/openapi/schema_builder.cpp Updates schemas for UpdateStatus and ConfigurationMetaData to declare nested x-medkit vendor extensions.
src/ros2_medkit_gateway/src/http/rest_server.cpp Fixes the documented OpenAPI response type for GET /apps/{id}/is-located-on to EntityList (matches actual handler behavior).
src/ros2_medkit_gateway/src/http/handlers/config_handlers.cpp Migrates configuration vendor keys from flat x-medkit-* to nested x-medkit objects.
src/ros2_medkit_gateway/include/ros2_medkit_gateway/updates/update_types.hpp Changes update_status_to_json() to emit nested x-medkit.phase instead of flat x-medkit-phase.
docs/api/rest.rst Updates update status example payload and vendor extension prose to the nested x-medkit.phase format.
.github/workflows/quality.yml Increases CI timeout from 45 to 60 minutes to avoid cancellations on cold-cache ASan runs.
Comments suppressed due to low confidence (1)

docs/api/rest.rst:1190

  • The docs claim GET /updates/{id}/status includes an error object when status is failed, but the gateway currently serializes error as a string (see update_status_to_json() and the OpenAPI UpdateStatus schema). Please update the example/wording here to match the actual wire format so clients don’t implement the wrong type.
   When ``status`` is ``failed``, an ``error`` object is included:

   .. code-block:: json

      {
        "status": "failed",
        "error": {
          "error_code": "internal-error",
          "message": "Download failed: connection timeout"
        }
      }

bburda added 2 commits April 27, 2026 19:11
Mirror of test_update_status_payload_uses_nested_x_medkit for the
configurations endpoint. The drift test cannot catch reintroduction of
the legacy flat x-medkit-source / x-medkit-node keys because the
ConfigurationMetaData schema is not closed (no additionalProperties:
false), so an explicit positive assertion locks in the new shape.

The test exercises temp_sensor's four declared ROS 2 parameters, which
covers the per-parameter emit path migrated by this PR.
fire_graph_callbacks() snapshotted GraphCallback copies and posted them to
the worker queue; the captured `this` from Ros2TopicDataProvider could be
invoked after the consumer's destructor freed members because
remove_graph_change() only cleared the slot for future snapshots.

Track per-slot in_flight counters: bump under graph_mtx_ when snapshotting,
drop them after the wrapper invokes each callback (or roll back if post()
rejects the wrapper during shutdown / queue full). remove_graph_change()
now waits on a condvar until in_flight[token] reaches zero, so callers can
safely tear down captured state. Adds a regression test that verifies the
synchronous drain semantic.

Surfaced by sanitizer-asan as a heap-use-after-free in
DataAccessManagerWithPublisherTest::TearDown -> ~Ros2TopicDataProvider ->
on_graph_change running on the executor worker.
Copy link
Copy Markdown
Collaborator

@mfaferek93 mfaferek93 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

A few comments :)

Comment thread src/ros2_medkit_gateway/src/openapi/schema_builder.cpp Outdated
Comment thread src/ros2_medkit_gateway/src/openapi/schema_builder.cpp Outdated
bburda added 2 commits April 28, 2026 20:29
…rift test

- configuration_metadata_schema(): declare ``x-medkit.node`` alongside
  ``source``. config_handlers.cpp emits both on every per-parameter entry
  in aggregated configurations; clients generated from the spec dropped
  ``node`` because it was undeclared. Add a static schema test that fails
  if either property goes missing - the integration drift test cannot
  catch this because configuration_metadata_schema leaves
  additionalProperties open by convention.

- update_status_schema(): rewrite the comment around the inner
  ``required: {phase}`` to spell out that ``phase`` is required only when
  x-medkit is present, not unconditionally.

- test_openapi_response_drift: replace the silent ``test_entity``
  fallback in _substitute_path_params with an explicit
  _MissingEntityType exception. The loop now skips paths whose entity
  type was not discovered and reports them in test output, instead of
  hitting 404s the loop dropped without validating. Added unit tests
  for the helper.

- test_openapi_response_drift: poll /docs until at least
  MIN_EXPECTED_PATHS paths are present, so a slow plugin registration
  cannot let the test read a partial spec and silently miss endpoints
  registered after the snapshot.

- test_openapi_response_drift: clarify class docstring on GET-only
  scope; POST/PUT/DELETE coverage stays out of scope.

- ros2_subscription_executor: strengthen on_graph_change() warning to
  describe the exact deadlock surface and the symptom (hung worker, no
  log line). Runtime detection would need thread_local TLS, which is
  banned in code linked into the gateway plugin MODULE.
…aggregate)

Rolling's gcc 14 / libstdc++ enforces the C++20 wording of [dcl.init.aggr]
that disqualifies a class as an aggregate when it has any user-declared
constructors - including ``= delete`` ones (rule tightened from C++17's
"user-provided" to C++20's "user-declared"). Even with
CMAKE_CXX_STANDARD=17 set, the libstdc++ headers/checks behave as C++20
on rolling, breaking ``LockedSubscriptionGuard{&mtx, sub, cg}`` because
the only candidates are the deleted copy/move constructors.

Add an explicit 3-arg constructor so brace-init resolves to direct-init
regardless of aggregate semantics across distros (Jazzy/Humble C++17 and
Rolling effective-C++20). Reproduces locally with ``g++ -std=c++20``.
Copy link
Copy Markdown
Collaborator

@mfaferek93 mfaferek93 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

+1

@bburda bburda merged commit 82fb2ee into main Apr 29, 2026
14 checks passed
@bburda bburda deleted the feat/issue-385-nest-x-medkit-vendor-fields branch April 29, 2026 09:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

Standardize vendor fields on SOVD-endpoint payloads as nested x-medkit object

3 participants